Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

perf(core): avoid recursive scope recalculation when TestBed.overrideModule is used #35454

Closed
wants to merge 1 commit into from
Closed

perf(core): avoid recursive scope recalculation when TestBed.overrideModule is used #35454

wants to merge 1 commit into from

Conversation

AndrewKushnir
Copy link
Contributor

Currently if TestBed detects that TestBed.overrideModule was used, transitive scopes are recalculated recursively for all modules and previously calculated data (stored in cache) is ignored. This behavior was introduced in #33787 and causes performance issues in case modules dependency tree is large. This commit updates the logic to avoid unnecessary recursive recalculation in nested modules.

Fixes #35395.

PR Type

What kind of change does this PR introduce?

  • Other... Please describe: perf improvement for Ivy TestBed.

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir AndrewKushnir added state: WIP target: patch This PR is targeted for the next patch release comp: ivy labels Feb 14, 2020
@ngbot ngbot bot modified the milestone: needsTriage Feb 14, 2020
@AndrewKushnir AndrewKushnir mentioned this pull request Feb 14, 2020
Closed
@AndrewKushnir AndrewKushnir marked this pull request as ready for review February 18, 2020 19:10
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Feb 18, 2020
@alxhub
Copy link
Member

alxhub commented Feb 19, 2020

Additionally, I think the commit message is a bit sparse. It should explain in detail under what circumstances unnecessary recalculation happens, and what's being changed to eliminate it.

@alxhub
Copy link
Member

alxhub commented Feb 19, 2020

Oh, weird, Github posted my comment twice.

@alxhub
Copy link
Member

alxhub commented Feb 20, 2020

Per offline discussion:

  1. the logic in this PR is correct: there is no need to clear scopes of any children modules of an overridden module, as they cannot be affected by the override. This is what the change accomplishes.
  2. the perf issue comes from a "diamond" problem, where module X is overridden which imports modules A and B, which both import module C. Under previous logic, module C gets its transitive deps recomputed multiple times, during the recompute for both A and B. For deep graphs this can be super costly.
  3. @AndrewKushnir will update the commit message to more thoroughly describe the perf issue, how it occurs under the current logic, and how the new logic fixes it.
  4. we might have another bug, where if module A imports B and B has overrides, module A's transitive scopes need to be recomputed as well. It's unclear if R3TestBed currently does this - @AndrewKushnir will write a test and see how we do on this case, but we should land this PR regardless.

Approved!

@AndrewKushnir AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Feb 21, 2020
@AndrewKushnir
perf(core): avoid recursive scope recalculation when TestBed.override…
4547030 
…Module is used

Currently if TestBed detects that TestBed.overrideModule was used for module X, transitive scopes are recalculated recursively for all modules that X imports and previously calculated data (stored in cache) is ignored. This behavior was introduced in angular#33787 to fix stale transitive scopes issue (cache was not updated if module overrides are present).

The perf issue comes from a "diamond" problem, where module X is overridden which imports modules A and B, which both import module C. Under previous logic, module C gets its transitive deps recomputed multiple times, during the recompute for both A and B. For deep graphs and big common/shared modules this can be super costly.

This commit updates the logic to recalculate ransitive scopes for the overridden module, while keeping previously calculated scopes of other modules untouched.
@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Feb 22, 2020
@AndrewKushnir
Copy link
Contributor Author

AndrewKushnir commented Feb 22, 2020 •
edited

@AndrewKushnir
Copy link
Contributor Author

FYI, Blueprint and Global TAP runs are successful, so marking this PR with "merge" label.

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: presubmit The PR is in need of a google3 presubmit labels Feb 23, 2020
@AndrewKushnir
Copy link
Contributor Author

MERGE ASSISTANCE: this PR is good to go in it's current state (see #35454 (comment)), followup actions will be performed outside of the scope of this PR. Thank you.

@AndrewKushnir
Copy link
Contributor Author

  1. we might have another bug, where if module A imports B and B has overrides, module A's transitive scopes need to be recomputed as well. It's unclear if R3TestBed currently does this - @AndrewKushnir will write a test and see how we do on this case, but we should land this PR regardless.

@alxhub I investigated this scenario and it looks like it works as expected and we already have a test that proves it. Thank you.

mhevery pushed a commit that referenced this pull request Feb 25, 2020
@AndrewKushnir @mhevery
perf(core): avoid recursive scope recalculation when TestBed.override…
349539e 
…Module is used (#35454)

Currently if TestBed detects that TestBed.overrideModule was used for module X, transitive scopes are recalculated recursively for all modules that X imports and previously calculated data (stored in cache) is ignored. This behavior was introduced in #33787 to fix stale transitive scopes issue (cache was not updated if module overrides are present).

The perf issue comes from a "diamond" problem, where module X is overridden which imports modules A and B, which both import module C. Under previous logic, module C gets its transitive deps recomputed multiple times, during the recompute for both A and B. For deep graphs and big common/shared modules this can be super costly.

This commit updates the logic to recalculate ransitive scopes for the overridden module, while keeping previously calculated scopes of other modules untouched.

PR Close #35454
@mhevery mhevery closed this in 0a1a989 Feb 25, 2020
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@IgorMinar IgorMinar IgorMinar left review comments

@alxhub alxhub alxhub approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

Unit Tests - Overriding large modules slows down createComponent
4 participants
@AndrewKushnir @alxhub @IgorMinar @googlebot